Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@luckysmg
Copy link
Contributor

@luckysmg luckysmg commented Nov 19, 2021

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#72742

The Android solution is here:
#29293

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@chinmaygarde
Copy link
Member

Ping @gaaclarke. Do you have the cycles to review this?

@gaaclarke
Copy link
Member

I'm not a fan of this change. It doesn't add any new functionality, just adds some convenience. That doesn't scale, users are able to implement convenience functions on their own side, there's no need to add them to the API.

@gaaclarke gaaclarke closed this Dec 2, 2021
@xster
Copy link
Member

xster commented Dec 3, 2021

I think a sensible argument for this one is that the owner of the FlutterEngineGroup lifecycle is outside of the vc lifecycle. That's expected. But the owner of the FlutterEngine (if it were to be manually injected) must also both 1- sit outside of the vc lifecycle and 2- monitor the vc lifecycle to destroy the engine when it's no longer needed. This could be cumbersome to maintain separately.

@gaaclarke
Copy link
Member

It's not really cumbersome, you just have to write

[[FlutterViewController alloc] initWithEngine:[_engineGroup makeEngine]]

versus

[[FlutterViewController alloc] initWithEngineGroup:_engineGroup]

I like that this advertises the feature a bit more, but I'd rather error on the side of a smaller combinatorial API since that's the only sane way forward. Unless of course the boilerplate becomes a burden. I don't see evidence that it is here.

@xster
Copy link
Member

xster commented Dec 7, 2021

ya, this SG. I was initially concerned about the manual burden of having to also keep track of the destruction of the VC to match to the destruction of the engine. But it seems like that's not the case since the group only holds pointers that will be removed when the owning VC deallocates the engine https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm#L88.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants